Master CI failure#60
Merged
Merged
Conversation
The syncPolymerRunColorMap$ effect (added in PR #58) uses getRunColorMap which transitively depends on the settings feature selector via getColorPalette. In the karma bundle test, when the settings feature state is not registered in a particular test's MockStore, selectSettingsState returns undefined, causing 'TypeError: Cannot read property settings of undefined' in the memoized selector chain. Fix 1 - settings_selectors.ts: Wrap the bare createFeatureSelector with a createSelector that falls back to initialState when the feature state is undefined. This prevents crashes in any test that evaluates settings-dependent selectors without registering the settings feature. Fix 2 - colorScale.ts: The readColorMap() function threw when window.__tbRunColorMap was not set. During tests and before the first NgRx effect fires, this map is absent. Changed to return null gracefully, skip domain population when the map is absent, and return a neutral fallback color (#808080) from getColor instead of throwing when a run is not in the domain. Co-authored-by: Samuel <samuel@knutsen.co>
Revert the overly-defensive approach. When window.__tbRunColorMap is not yet seeded (race between runsStore listener and the NgRx effect), fall back to the original static palette assignment so the domain is always fully populated and getColor throws for actual programming errors. When the shared color map IS available but a run is missing, log console.error so the problem is visible. Co-authored-by: Samuel <samuel@knutsen.co>
When the shared color map exists but is missing a specific run, fall back to the palette color for that entry (and log the error). Previously the identifier was set to undefined, which would cause getColor to return undefined instead of a hex string. Co-authored-by: Samuel <samuel@knutsen.co>
|
Cursor Agent can help with this pull request. Just |
Preview Deployment
Details
|
Co-authored-by: Samuel <samuel@knutsen.co>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation for features / changes
This PR fixes a critical CI failure on master (
TypeError: Cannot read property 'settings' of undefined) that occurred after merging PR #58. The failure was caused by a race condition and missing NgRx feature state in certain test environments, specifically whenRunsEffectsandFeatureFlagEffectswere initialized without theSettingsModulebeing loaded.Technical description of changes
settings_selectors.ts: TheselectSettingsStatefeature selector is now wrapped withcreateSelectorto provideinitialStateas a fallback. This prevents crashes when the settings feature state is accessed before its module is registered, aligning with standard NgRx patterns.colorScale.ts: TheColorScalecomponent, which was updated in PR Run selection persistence #58 to depend onwindow.__tbRunColorMap(seeded by an NgRx effect), is made more resilient to timing issues.readColorMap()now returnsnullifwindow.__tbRunColorMapis not yet available, instead of throwing an error.setDomain()falls back to the original static color palette whenwindow.__tbRunColorMapisnull. If the map is present but a run is missing, aconsole.erroris logged to indicate a potential bug.getColor()continues to throw an error if a run is requested that is not in the domain, ensuring actual programming errors are not silently swallowed.Screenshots of UI changes (or N/A)
N/A
Detailed steps to verify changes work correctly (as executed by you)
TypeError: Cannot read property 'settings' of undefinedandsetFeatureFlagsof undefined) after PR Run selection persistence #58 merge.settingserror togetRunColorMapselector chain evaluating beforeSettingsModulewas registered in test contexts.settings_selectors.tsto provideinitialStatefallback.setFeatureFlagserror andcolorScale.tsissues to race conditions where Polymer elements orwindow.__tbRunColorMapwere not ready when accessed.colorScale.tsto gracefully handlewindow.__tbRunColorMapnot being seeded yet, falling back to static palette, while still throwing for actual missing runs.console.errorinsetDomainwould not cause test failures but would highlight real bugs in production.Alternate designs / implementations considered (or N/A)
Initially, a more aggressive error silencing approach was considered for
colorScale.ts(e.g., returning a default color fromgetColorif a run was not found). However, this was reverted in favor of a more robust solution that distinguishes between legitimate timing issues (falling back to a static palette) and actual programming errors (throwing fromgetColoror loggingconsole.errorfor missing runs when the map is expected to be ready). This ensures that real bugs are not silently hidden.